Skip to content

refactor: reduce complexity of generate_mcpg_config in compile/common.rs#322

Merged
jamesadevine merged 2 commits into
mainfrom
refactor/reduce-complexity-generate-mcpg-config-41ba926acb5bd784
Apr 25, 2026
Merged

refactor: reduce complexity of generate_mcpg_config in compile/common.rs#322
jamesadevine merged 2 commits into
mainfrom
refactor/reduce-complexity-generate-mcpg-config-41ba926acb5bd784

Conversation

@github-actions

Copy link
Copy Markdown
Contributor

Summary

Reduces the cognitive complexity of generate_mcpg_config from 31 (flagged by clippy::cognitive_complexity) to below 25 (no longer flagged) by extracting focused helper functions.

What was complex

The original function contained a large for (name, config) in &front_matter.mcp_servers loop with:

  • Two levels of nested if let Some(opts) = options { if let Some(container) = ... { ... } else if let Some(url) = ... { ... } } branching
  • Seven repetitive if vec.is_empty() { None } else { Some(vec.clone()) } patterns spread across the container and HTTP branches
  • Inline validation calls (validate_container_image, validate_mount_source, validate_docker_args, warn_potential_secrets) mixed with construction logic

What changed

Five private helpers were extracted:

Helper Purpose
nonempty_vec / nonempty_map Eliminate 7 repetitive empty-check-to-Option conversions
validate_stdio_mcp Group all validation calls for container-based MCPs
build_stdio_mcpg_server Construct McpgServerConfig for container (stdio) MCPs
build_http_mcpg_server Construct McpgServerConfig for HTTP MCPs
try_add_user_mcp Handle the full lifecycle of one user-defined MCP entry: reserved-name guard, name validation, enabled check, dispatch to stdio or HTTP branch

generate_mcpg_config itself now contains only the top-level orchestration — iterating extensions, then calling try_add_user_mcp once per entry.

Verification

  • cargo test — all tests pass
  • cargo clippy --all-targets --all-features — clean (no new warnings)
  • cargo clippy ... -W clippy::cognitive_complexitygenerate_mcpg_config no longer appears in output

No public API signatures or observable behaviour changed.

Warning

⚠️ Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • dev.azure.com

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "dev.azure.com"

See Network Configuration for more information.

Generated by Cyclomatic Complexity Reducer · ● 1.6M ·

Extract helper functions to reduce cognitive complexity from 31 to below 25:

- nonempty_vec / nonempty_map: eliminate 7 repetitive if-is-empty patterns
- validate_stdio_mcp: groups all validation calls for container MCPs
- build_stdio_mcpg_server: constructs McpgServerConfig for container MCPs
- build_http_mcpg_server: constructs McpgServerConfig for HTTP MCPs
- try_add_user_mcp: handles the full lifecycle of one user-defined MCP entry
  (reserved-name check, name validation, enabled check, dispatch to stdio/http)

generate_mcpg_config itself now contains only the top-level orchestration loop
calling try_add_user_mcp per entry.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@jamesadevine

Copy link
Copy Markdown
Collaborator

/rust-review

@github-actions

github-actions Bot commented Apr 24, 2026

Copy link
Copy Markdown
Contributor Author

Rust PR Reviewer completed successfully!

@github-actions

Copy link
Copy Markdown
Contributor Author

🔍 Rust PR Review

Summary: Looks good — clean, correct refactoring with no behavioral changes.

Findings

⚠️ Suggestions

  • validate_stdio_mcp (line 1156): The function re-extracts container via opts.container.as_deref().unwrap_or(""), but it's only ever called from the if let Some(container) = &opts.container branch in try_add_user_mcp, so the unwrap_or("") is dead code. Passing container: &str as a parameter (already extracted at the call site) would be more explicit and eliminate this:

    fn validate_stdio_mcp(name: &str, container: &str, opts: &McpOptions) {
        for w in validate::validate_container_image(container, name) { ...
  • nonempty_vec doc comment (line 1140): Doc says Some(v.clone()) but the body uses v.to_vec(). Semantically identical, but the comment is technically inaccurate. Minor.

✅ What Looks Good

  • Behavioral equivalence is exact — I traced through every branch of the old generate_mcpg_config loop and confirmed try_add_user_mcp reproduces it faithfully, including the McpConfig::Enabled(true) → "no container or url" warning path, the extension-already-registered early return, and the error bail for invalid names propagating through ?.
  • nonempty_vec / nonempty_map are a genuine improvement — the 7 repetitive empty-check-to-Option patterns in the original were error-prone to maintain.
  • build_stdio_mcpg_server and build_http_mcpg_server are pure constructors with no hidden control flow, easy to test in isolation if desired.
  • Error handling is unchanged: anyhow::bail! in try_add_user_mcp propagates correctly via ? at the call site.

Generated by Rust PR Reviewer for issue #322 · ● 402.2K ·

@jamesadevine

Copy link
Copy Markdown
Collaborator

@copilot address suggestions

Copilot AI commented Apr 24, 2026

Copy link
Copy Markdown
Contributor

@copilot address suggestions

Addressed both suggestions in d61a010:

  1. validate_stdio_mcp now accepts container: &str as an explicit parameter (removing the dead unwrap_or("") re-extraction). The call site in try_add_user_mcp passes the already-bound container from the if let Some(container) = &opts.container branch.

  2. nonempty_vec doc comment updated from Some(v.clone()) to Some(v.to_vec()) to match the body.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • dev.azure.com
    • Triggering command: /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-6cf427a7041718b7 /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-6cf427a7041718b7 /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-756301ed8a6501c8.0llthg/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/cli_tests-2a110a478d07aadb.1o01d2wx4ogdbuatinxctq7c2.0oex7ro.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-756301ed8a6501c8.0mw6gp/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/cli_tests-2a110a478d07aadb.20obb1y0jbhfh7k5w52g8szj8.0oex7ro.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-756301ed8a6501c8.0otj52/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/cli_tests-2a110a478d07aadb.24pn47b32n8edawr9qj7cz8s2.0oex7ro.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-756301ed8a6501c8.0p5rgi/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/cli_tests-2a110a478d07aadb.2n8mpj2icd8ncb480guogecyo.0oex7ro.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-756301ed8a6501c8.0shqrm/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/cli_tests-2a110a478d07aadb.2y5sqdhn8r3wd8hlyypyucgqc.0oex7ro.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-756301ed8a6501c8.0srycs/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/cli_tests-2a110a478d07aadb.38s2uunwgecs63dy42y1dy75e.0oex7ro.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-756301ed8a6501c8.0szu9d/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/cli_tests-2a110a478d07aadb.3yvzzqjqxq61a7gxx9uk8i1ks.0oex7ro.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-756301ed8a6501c8.0uqxor/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/cli_tests-2a110a478d07aadb.3zuv8i258hizjdjqioybdpjlo.0oex7ro.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-756301ed8a6501c8.0v34js7cex�� /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-756301ed8a6501c8.0vtev9wioausy59e9h9rdmqpn.0yt9go1.rcgu.o /home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/ado_aw-756301ed8a6501c8.0x1b3tg2bf3blnac8sw66cipv.0yt9go1.rcgu.o g/de�� lib/rustlib/x86_/home/REDACTED/.rustup/toolchains/stable-x86_64-REDACTED-linux-gnu/lib/rustlib/x86_cc lib/rustlib/x86_/home/REDACTED/.rustup/toolchains/stable-x86_64-REDACTED-linux-gnu/lib/rustlib/x86_-m64 bin/rustc res-0d1f7d618793cc res-0d1f7d618793-m64 res-0d1f7d618793/home/REDACTED/work/ado-aw/ado-aw/target/debug/deps/rustc2jQJXh/symbols.o bin/rustc (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

Copilot AI requested a review from jamesadevine April 24, 2026 21:55
@jamesadevine jamesadevine marked this pull request as ready for review April 25, 2026 05:53
@jamesadevine jamesadevine merged commit f597498 into main Apr 25, 2026
@github-actions github-actions Bot mentioned this pull request Apr 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants